Prevent NullReferenceException in SentryTraceHeader Parsing #3745
Conversation
Removed the `?` symbol from the test variable as nullable reference types are not enabled in the test project
Improved the XML documentation for the `Parse` method in `SentryTraceHeader`. Added details on parameter format, return conditions, and exception handling to clarify usage and expected input structure
Victorvhn
requested review from
bitsandfoxes and
jamescrosswell
as code owners
|
Awesome, thanks for the PR @Victorvhn ! |
Hey James, thanks for the quick response! I just pushed a new change to the Always happy to help, man! |
|
Edit: |
|
Should this go into the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3745 +/- ##
==========================================
+ Coverage 75.73% 77.03% +1.30%
==========================================
Files 357 386 +29
Lines 13466 14380 +914
Branches 2671 2874 +203
==========================================
+ Hits 10198 11078 +880
- Misses 2593 2599 +6
- Partials 675 703 +28 ☔ View full report in Codecov by Sentry. |
Good question, I was seeing already merged PRs, and I noticed they were merged to the main branch. I think it's open to you guys |
I think the main branch is fine. There aren't any dependencies on anything from the version-5.0.0 branch for this fix. @Victorvhn aplogies for the breaking builds. Once this gets merged into main we can update this branch from main and the build issues should be resolved: |
I was thinking about this being a breaking change since the returning traceheader is now nullable. |
Actually, I guess it was already expected. The places I saw it being used were already expecting nullable values there, but I could be wrong |
Ahhh, I see. That's why all the verify tests changed. Yes, in that case I think we should put it on the version-5.0.0 branch. @Victorvhn would you be able to make a new PR with these changes, forking from |
Sure, I just opened this new one: #3757 Should I close this one? |
|
Replaced by #3757 (targets version-5.0.0) |
Victorvhn
deleted the
fix/prevent-null-reference-sentry-trace-header-parser
branch
3 participants
Hey y'all,
I recently encountered an issue where nearly every API request threw an exception after integrating Sentry with OpenTelemetry in my APIs. This issue only affected my environment, likely due to having Just My Code disabled in debugging options.
Upon investigation, I discovered that a null value was being passed to the
Parsemethod inSentryTraceHeader.cs. The method was attempting to call.Split()on this null value, leading to anNullReferenceException.To address this, I added a null check in the Parse method before calling
.Split().Please review the changes when you have a chance and let me know if there are any additional adjustments or improvements you’d like to see. Thank you!